[feat][node] Generalize request correlation by thep2p · Pull Request #124 · mathcrypt-research/skipgraph-rust

TheP2P (thep2p) · 2026-08-13T03:30:23Z

Generalizes BaseNode request correlation so one nonce-keyed map serves every message
type, and adds the phase-0 join bootstrap query on top of it.

src/node/waiter.rs (new). Waiter carries one variant per message type, a
blocking SyncSender for search_by_id and a oneshot::Sender for get_max_level,
because the two callers differ in concurrency shape. WaiterGuard ties map cleanup to
request scope via Drop, the only cleanup an async caller cannot skip by dropping its
future mid-await.
get_max_level. Asks an introducer for its highest populated lookup-table level
under a caller-supplied timeout.
Routing. RetMaxLevelOp resolves by nonce, and a wrong-variant or expired entry is
a no-op.

Tests cover the single-request resolve, the timeout plus map cleanup, and a blocking and
an async waiter live in the map at once.

Closes #88

Copilot

Pull request overview

Generalizes node request correlation to support multiple response types.

Changes:

Adds typed search and max-level waiters with RAII cleanup.
Implements asynchronous max-level requests and response routing.
Adds concurrency and timeout tests; updates private-file ignores.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.

File	Description
`src/node/waiter.rs`	Defines waiter variants and cleanup guard.
`src/node/base_node.rs`	Adds max-level requests, routing, and tests.
`src/node/mod.rs`	Registers the waiter module.
`.gitignore`	Renames the private-material ignore path.

Suppressed comments (1)

src/node/base_node.rs:305

This similarly removes any waiter before checking its kind. A RetMaxLevelOp with a live search nonce silently evicts the Search sender, so the blocking search fails instead of remaining pending. Preserve entries whose variant does not match this response type.

                let waiter = self
                    .request_id_map
                    .lock()
                    .expect("mutex was poisoned by a previous panic")
                    .remove(&res.nonce);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

TheP2P (thep2p) · 2026-08-16T18:50:46Z

+        if let Err(e) = self.net.send_event(
+            introducer,
+            GetMaxLevelOp(MaxLevelReq {
+                nonce,
+                origin: self.core.id(),
+            }),


Correct that the arm is missing today, but that's intentional scope for this PR, not an oversight. get_max_level is still #[allow(dead_code)] with a TODO naming the follow-up wiring, its own doc comment calls it a latency optimization rather than a correctness requirement, and the introducer-side handler (computing the highest populated level plus the RetMaxLevelOp reply) is tracked as separate follow-up work sequenced after this one. This PR's scope is the request-correlation mechanism (the asking side), not the new Core operation the responder needs.

TheP2P (thep2p) · 2026-08-16T18:38:39Z

@@ -209,10 +285,38 @@ impl EventProcessorCore for BaseNode {
                    .lock()
                    .expect("mutex was poisoned by a previous panic")
                    .remove(&res.nonce);


Fixed: only remove the map entry when it's actually a Search waiter, so a mismatched-variant response no longer evicts a live MaxLevel waiter.
e76c978

The mirrored case at line 305 (RetMaxLevelOp evicting a live Search waiter) had the same bug, fixed separately:
8080881

TheP2P (thep2p) · 2026-08-16T18:38:36Z

+    #[allow(dead_code)] // TODO: remove once phase-0 bootstrap is wired into join orchestration.
+    pub(crate) async fn get_max_level(
+        &self,
+        introducer: Identifier,
+        timeout: Duration,
+    ) -> anyhow::Result<LookupTableLevel> {
+        let span = tracing::trace_span!("get_max_level", introducer = ?introducer);
+        let _enter = span.enter();


Fixed: wraps the future in .instrument(span) instead of holding enter() across the await, matching the pattern already used in core/context/mod.rs.
df3446e

TheP2P (thep2p) · 2026-08-16T18:38:41Z

+        // cleans up the map entry on every exit path, including cancellation. Never read
+        // (its only job is running `Drop` at end of scope), hence the `_` prefix.
+        let _guard = WaiterGuard::new(nonce, self.request_id_map.clone());


Added a cancellation test: spawns get_max_level, aborts it mid-flight before any reply is delivered, and asserts the waiter-map entry is cleaned up.
8cda86c

TheP2P (thep2p) · 2026-08-16T19:03:40Z

Claude (@claude) review

claude · 2026-08-16T19:03:53Z

Claude encountered an error —— View job

I'll analyze this and get back to you.

claude · 2026-08-16T20:15:09Z

Claude finished TheP2P (@thep2p)'s task —— View job

Code Review

Reviewed the request-correlation generalization. Overall this is well-crafted work: the Waiter/WaiterGuard design is sound, the concurrency reasoning is careful, and the doc comments explain the why (span instrumentation, drop-on-cancel cleanup, poisoned-lock handling) unusually well. All five prior Copilot comments have been addressed. A few observations below.

Correctness / design

Variant-preserving eviction is now correct (base_node.rs:294-298 and 314-324). Both response arms now get()-and-check the variant before remove(), so a mismatched-nonce response no longer evicts a live sibling waiter. Good fix, and mirrored consistently on both arms.
GetMaxLevelOp responder arm is intentionally absent — agreed this is acceptable for this PR's scope (the asking side). get_max_level is #[allow(dead_code)] with a TODO, and the doc frames it as a latency optimization. One suggestion: the incoming GetMaxLevelOp currently falls through to the _ => arm which returns Err("unsupported event payload type") (base_node.rs:340-343). Since the responder is deliberately deferred, consider whether an introducer receiving this today should hard-error vs. log-and-noop. As-is it's fine, but worth a tracking note so the follow-up doesn't leave it erroring in a live topology.

Minor / nits

waiter.rs:8-14 — the doc has a sentence-fragment break: "...not a map per type. Because the lock protects one logical entity, ...". The Because clause is left dangling. Minor prose cleanup.
search_by_id cleanup asymmetry (base_node.rs:119-146) — the blocking path still uses manual .remove() in each error branch rather than a WaiterGuard. The waiter.rs doc explicitly calls this out as intentional ("search_by_id's existing manual removals are left as-is"), so this is a documented choice, not a defect. Noting only that a future unification could reduce the two cleanup styles to one.
send_event failure on the max-level path (base_node.rs:199-207) returns before the _guard goes out of scope — good, the guard still cleans up the just-inserted entry on this early return. Verified the entry is inserted (line 193) before the guard is created (line 197), so drop order covers it.

Tests

test_get_max_level_resolves, test_get_max_level_times_out_and_cleans_up, test_concurrent_requests_of_different_types_resolve_independently, and the new test_get_max_level_cleans_up_on_cancellation give strong coverage: single resolve, timeout+cleanup, sibling-independence, and drop-on-abort. The cancellation test correctly polls the shared map (not just nonce capture) to confirm registration before aborting, and uses bounded timeout loops rather than sleeps — consistent with the project's async-wait convention.
Nice touch confirming request_id_map.is_empty() after both timeout and abort paths, which is exactly what proves the WaiterGuard did its job.

Unrelated diffs: identifier.rs (drops redundant & on hex::encode args) and .gitignore (internal/ → .brain/) are cosmetic and harmless, though technically out of this PR's stated scope.

Nothing blocking. LGTM once the minor prose nit (#3) is optionally cleaned up.

Note: I was unable to run cargo test/clippy in this environment (command approval not granted), so the above is from static review only.
• Branch: thep2p/88-request-correlation

TheP2P (thep2p) · 2026-08-17T01:18:48Z

Fixed the dangling sentence in the Waiter doc comment: 9bea5ab

The other item (GetMaxLevelOp falling through to a hard error) is accurate as described but not actionable right now, there's no production transport or live caller yet, and the real handler is already tracked by #91/#86, sequenced right after this PR.

TheP2P (thep2p) added 3 commits August 9, 2026 13:01

[cleanup] Rename internal/ to .brain/ in gitignore

665498c

[feat][node] Generalize request correlation

8a0a032

Merge branch 'main' into thep2p/88-request-correlation

7dabf92

TheP2P (thep2p) requested a balanced review from Copilot August 13, 2026 03:35

Copilot started reviewing on behalf of TheP2P (thep2p) August 13, 2026 03:36 View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

TheP2P (thep2p) requested a review from S T (staheri14) August 13, 2026 04:16

TheP2P (thep2p) added 6 commits August 13, 2026 10:02

[cleanup][core] Drop redundant refs in format args

c010e08

[fix][node] Drop dangling doc link in waiter.rs

08d6e94

[fix][node] Fix span leak in get_max_level

df3446e

[fix][node] Guard search-response waiter removal

e76c978

[fix][node] Guard max-level response removal

8080881

[feat][test] Add get_max_level cancellation test

8cda86c

[cleanup][node] Fix dangling sentence in Waiter doc

9bea5ab

Navigation Menu

Uh oh!

[feat][node] Generalize request correlation - #124

[feat][node] Generalize request correlation#124
TheP2P (thep2p) wants to merge 10 commits into
mainfrom
thep2p/88-request-correlation

TheP2P (thep2p) commented Aug 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

TheP2P (thep2p) Aug 16, 2026

Uh oh!

TheP2P (thep2p) Aug 16, 2026

Uh oh!

TheP2P (thep2p) Aug 16, 2026

Uh oh!

TheP2P (thep2p) Aug 16, 2026

Uh oh!

Uh oh!

TheP2P (thep2p) commented Aug 16, 2026

Uh oh!

claude Bot commented Aug 16, 2026 •

edited

Loading

Uh oh!

claude Bot commented Aug 16, 2026 •

edited

Loading

Uh oh!

TheP2P (thep2p) commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

TheP2P (thep2p) commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

TheP2P (thep2p) Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

TheP2P (thep2p) Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

TheP2P (thep2p) Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

TheP2P (thep2p) Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheP2P (thep2p) commented Aug 16, 2026

Uh oh!

claude Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Correctness / design

Minor / nits

Tests

Uh oh!

TheP2P (thep2p) commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TheP2P (thep2p) commented Aug 13, 2026 •

edited

Loading

claude Bot commented Aug 16, 2026 •

edited

Loading

claude Bot commented Aug 16, 2026 •

edited

Loading